Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[10.x] Stop adding constraints twice on *Many to *One relationships via one() #46575

Merged
merged 4 commits into from
Mar 25, 2023
Merged

[10.x] Stop adding constraints twice on *Many to *One relationships via one() #46575

merged 4 commits into from
Mar 25, 2023

Conversation

cosmastech
Copy link
Contributor

@cosmastech cosmastech commented Mar 24, 2023 •
edited
Loading

@siarheipashkevich reported that constraints are duplicated when being converted from HasMany/HasManyThrough/MorphMany to HasOne/HasOneThrough/MorphOne

This PR will stop adding those constraints twice by employing the Relation::noConstraint() method (which was a TIL moment and I must say, is 🔥)

@cosmastech cosmastech marked this pull request as draft March 24, 2023 13:05
@cosmastech cosmastech mentioned this pull request Mar 24, 2023
Merged
@cosmastech cosmastech marked this pull request as ready for review March 24, 2023 13:19
@cosmastech cosmastech changed the title create new HasOne from HasMany without constraints [10.x] create new HasOne from HasMany without constraints Mar 24, 2023
@cosmastech cosmastech changed the title [10.x] create new HasOne from HasMany without constraints [10.x] Stop adding constraints twice on *Many to *One relationships via one() Mar 24, 2023
@siarheipashkevich
Copy link
Contributor

@cosmastech will it save additional conditions?

$this->shifts()->one()->opened() . opened is a scope.

@cosmastech
Copy link
Contributor Author

@cosmastech will it save additional conditions?

$this->shifts()->one()->opened() . opened is a scope.

@siarheipashkevich It should just ignore the constraints. Was the opened() scope not applying previously?

@siarheipashkevich
Copy link
Contributor

@cosmastech please take a look at my comment and log
#46443 (comment)

we can see that the scope has been applied and it's ok

@cosmastech
Copy link
Contributor Author

cosmastech commented Mar 24, 2023 •
edited
Loading

@cosmastech please take a look at my comment and log

#46443 (comment)

we can see that the scope has been applied and it's ok

The scope is applied after the fact so it should not be a problem.

Edit: even a scope applied in the original HasMany should be applied when you call one()

@taylorotwell taylorotwell merged commit 034d849 into laravel:10.x Mar 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cosmastech @siarheipashkevich @taylorotwell